[EC-775] [Technical Dependency] Refactor Vault Filters to be routable by coroiu · Pull Request #4733 · bitwarden/clients

coroiu · 2023-02-13T12:24:56Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR introduces an temporary bridge between URL filtering and the old state-in-code method. It enables us to move forward with refactoring the table and the navigation, and allow us to work on them separately. This is great for smaller PRs and more efficient coding.

Code changes

apps/web/src/vault/individual-vault/vault-filter/components/vault-filter.component.ts: Remove the reverse data flow. I.e. let the data flow one-way into the component. Also remove the removeInvalidCollectionSelection functionality because it's not needed anymore.
apps/web/src/vault/org-vault/vault-filter/vault-filter.component.ts: Same as individual vault component.
apps/web/src/vault/individual-vault/vault-filter/services/abstractions/vault-filter.service.ts: Add ability to get a cipherTypeTree which is needed to be able to populate the bridge filter model with tree nodes.
apps/web/src/vault/individual-vault/vault-filter/services/vault-filter.service.ts: Implementation of the above.
apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter.service.ts: The new filter service that builds and emits the new filter model based on URL params. Also contains a method for generating routes.
apps/web/src/vault/individual-vault/vault-filter/shared/models/routed-vault-filter.model.ts: The new model used to represent filters. Very basic, only primitives.
apps/web/src/vault/[org and individual]/vault.component.ts: Fetch active filters from the new services. Also remove applyVaultFilter which used direct component references to reload the vault-items.component
apps/web/src/vault/individual-vault/vault-items.component.ts: Reload ciphers when a new activeFilter is set by the parent component, instead of relying of direct function calls.

Temporary compatibility layer

This layer is used to temporary bridge between URL filtering and the old state-in-code method. This should be removed after we have refactored the vault-items.component and introduced vertical navigation (which will refactor the vault-filter.component).

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts: Listens to both the new RoutedVaultFilterService and the old VaultFilterService. When a new filter is emitted the service uses the ids to find the corresponding tree nodes needed for the old VaultFilter model. It then emits a bridge model that contains this information.
apps/web/src/vault/individual-vault/vault-filter/shared/models/routed-vault-filter-bridge.model.ts: This model supplies legacy code with the old state-in-code models saved as tree nodes. It can also receive requests to select a new tree node by using setters. However instead of just replacing the tree node models, it requests a URL navigation, thereby bridging between legacy and URL filtering.

Screenshots

Nothing has really changed graphically. Works just like usual.

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

…-filters-to-be-routable

Had to remove these becuase they were causing double navigations on each click.

vault-filter.component was pushing up old filter models which would sometimes overwrite new filter models that came from the routed filter service.

jlf0dev

Everything looks good to me, I just had some minor suggestions for the code. I'll also hold off approving until @eliykat takes a look as well.

jlf0dev · 2023-02-16T16:21:12Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+
+        if (filter.type !== undefined && filter.type === "trash") {
+          legacyFilter.selectedCipherTypeNode = {
+            node: { id: "trash", name: "", icon: "", type: "trash" },


minor suggestion: you could use the TreeNode constructor here:

legacyFilter.selectedCipherTypeNode = new TreeNode<CipherTypeFilter>({ id: "trash", name: "", type: "trash", icon: ""}, null);

we really should refactor this constructor to be more useful, I don't even think this will save you that many characters 😄

My question was going to be whether we could find the existing Trash node, or export & import the Trash node object, rather than recreating it. It looks like it's currently declared in the VaultFilterComponent though, which I guess we don't really want to pull in to a service.

In any case I agree you should use a real TreeNode object, not really to save any characters, just to be properly typesafe.

@jlf0dev Good point, will update!

@eliykat Yeah that was exactly the issue, I contemplated moving it from VaultFilterComponent to VaultFilterService but I really wanted to keep the modifications to a minimum, so this is what I ended up with it.

lgtm but I'll let @jlf0dev resolve the thread when he's happy

jlf0dev · 2023-02-16T16:23:04Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+
+        const bridgeModel = new RoutedVaultFilterBridge(filter, legacyFilter, this);
+
+        return bridgeModel;


suggestion: unnecessary variable here, just return the model

Huh, I wonder why I did this. Must've been a console.log here at some point 😅

jlf0dev · 2023-02-16T16:23:56Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+import { RoutedVaultFilterService } from "./routed-vault-filter.service";
+
+@Injectable()
+export class RoutedVaultFilterBridgeService {


suggestion: you could add a jsdoc comment here explaining the purpose of the bridge service

Something I'm wondering about 😁 is it transitional? If so, do we need a tech debt ticket to remove the old services? I assume that when we build the new filter list in the left-hand nav, they'll be routing directly to the query URL rather than going via a service, and then it'll be much simpler to clean up the old code. This feels like something EC pod should do as part of VVR, before handing off to Vault team.

Note that we also have DeprecatedVaultFilterService floating around 🤯

EDIT: ok, after reviewing the rest of the code, (I think) I understand that this is basically a wrapper around existing logic to implement routing with a minimum of code changes. The downside is that it adds complexity in the short term, but I'm OK with this if we're deprecating it quickly after VVR (because the new components won't use it).

@jlf0dev Sure I can add the file descriptions I've used in the PR description field into the code!

@eliykat Yes exactly, this is a transitional class. I'm glad that it becomes apparent when reading the code, means it's not too complex/confusing. As I mention in the Objective part of the PR description:

This PR introduces an temporary bridge between URL filtering and the old state-in-code method. It enables us to move forward with refactoring the table and the navigation, and allow us to work on them separately.

You have a good point, maybe we should add a TD ticket, but really all of this should be removed as part of VVR

Instead of a TD ticket I added a task to the VVR story instead EC-208 [Admin Console] Add Filters under Left Nav

Looks good, and serves me right for not reading your PR description properly 🙈
I'll let @jlf0dev mark this thread resolved once he's had a chance to look at it

jlf0dev · 2023-02-16T16:42:26Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+    node: TreeNode<T>,
+    id: string
+  ): TreeNode<T> | undefined {
+    if (node.node.id === id) {


comment: we need to rename the node to object or something at some point 😄

There's probably some background discussion here that I'm not familiar with, but - object seems worse! Everything's an object! 😅 and Object is a keyword.

Haha yeah, hopefully we'll get rid of the TreeNode completely and not have to worry about it :D

jlf0dev · 2023-02-16T16:43:51Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter.service.ts

+import { RoutedVaultFilterModel } from "../shared/models/routed-vault-filter.model";
+
+@Injectable()
+export class RoutedVaultFilterService implements OnDestroy {


suggestion: a comment here might be helpful as well. The name RoutedVaultFilterService makes sense, but only if you understand why we need it

eliykat

Great PR, 10/10 would review again.

This will also need to be implemented for other clients (particularly desktop) after we've "perfected" it in the web vault. (and by "we" I mean the vault team ;) Can you please create a linked tech debt ticket for that?

eliykat · 2023-02-17T04:05:36Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+
+        if (filter.type !== undefined && filter.type === "trash") {
+          legacyFilter.selectedCipherTypeNode = {
+            node: { id: "trash", name: "", icon: "", type: "trash" },


My question was going to be whether we could find the existing Trash node, or export & import the Trash node object, rather than recreating it. It looks like it's currently declared in the VaultFilterComponent though, which I guess we don't really want to pull in to a service.

In any case I agree you should use a real TreeNode object, not really to save any characters, just to be properly typesafe.

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

eliykat · 2023-02-17T04:20:37Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+import { RoutedVaultFilterService } from "./routed-vault-filter.service";
+
+@Injectable()
+export class RoutedVaultFilterBridgeService {


Something I'm wondering about 😁 is it transitional? If so, do we need a tech debt ticket to remove the old services? I assume that when we build the new filter list in the left-hand nav, they'll be routing directly to the query URL rather than going via a service, and then it'll be much simpler to clean up the old code. This feels like something EC pod should do as part of VVR, before handing off to Vault team.

Note that we also have DeprecatedVaultFilterService floating around 🤯

EDIT: ok, after reviewing the rest of the code, (I think) I understand that this is basically a wrapper around existing logic to implement routing with a minimum of code changes. The downside is that it adds complexity in the short term, but I'm OK with this if we're deprecating it quickly after VVR (because the new components won't use it).

apps/web/src/vault/individual-vault/vault.component.ts

eliykat · 2023-02-17T04:57:46Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+    node: TreeNode<T>,
+    id: string
+  ): TreeNode<T> | undefined {
+    if (node.node.id === id) {


There's probably some background discussion here that I'm not familiar with, but - object seems worse! Everything's an object! 😅 and Object is a keyword.

apps/web/src/vault/org-vault/vault-filter/vault-filter.component.ts

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter.service.ts

...eb/src/vault/individual-vault/vault-filter/shared/models/routed-vault-filter-bridge.model.ts

apps/web/src/vault/individual-vault/vault-filter/shared/models/routed-vault-filter.model.ts

coroiu · 2023-02-17T14:34:22Z

@eliykat Ticket already seems to exist EC-517 [Tech Debt] Refactor Vault Filter in Browser and Desktop

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter.service.ts

eliykat · 2023-02-20T02:15:45Z

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

+
+        if (filter.type !== undefined && filter.type === "trash") {
+          legacyFilter.selectedCipherTypeNode = {
+            node: { id: "trash", name: "", icon: "", type: "trash" },


lgtm but I'll let @jlf0dev resolve the thread when he's happy

eliykat

I think I've responded to everything above :)

This is needed due to how defaults used to work when using `state-in-code`. We really want to get rid of this type of logic going forward.

...eb/src/vault/individual-vault/vault-filter/shared/models/routed-vault-filter-bridge.model.ts

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter.service.ts

apps/web/src/vault/individual-vault/vault-filter/services/routed-vault-filter-bridge.service.ts

It's better that we circle back to this type of navigationt when we're working on the VVR and have more knowledge about how this is supposed to work.

Refactor follows feedback from PR review

…-filters-to-be-routable

eliykat

Great work! Let's get it to QA

…-filters-to-be-routable

coroiu added 13 commits February 3, 2023 15:18

[EC-775] feat: add compatibility layer from #4154

e8e8f7b

[EC-775] fix: ciphers not reloading on filter change

f9888b6

[EC-775] feat: add support for cipher types

9f0dcfe

[EC-775] feat: implement organization switching

096e7fd

Merge branch 'master' into EC-775-technical-dependency-refactor-vault…

fcc49a2

…-filters-to-be-routable

[EC-775] feat: remove invalid folder and collection checks

b2c341f

Had to remove these becuase they were causing double navigations on each click.

[EC-775] fix: fix reverse data flow race condition

7594233

vault-filter.component was pushing up old filter models which would sometimes overwrite new filter models that came from the routed filter service.

[EC-775] fix: No folder use-case not working

19c5bab

[EC-775] feat: make navigation behave like master

4eaeb9b

[EC-775] feat: add support for trash

4452ddd

[EC-775] chore: simplify findNode

1e13859

[EC-775] feat: add support for org vault

0b6b075

[EC-775] feat: add support for orgId in path

31944d0

coroiu marked this pull request as ready for review February 15, 2023 14:27

coroiu requested a review from a team as a code owner February 15, 2023 14:27

eliykat self-requested a review February 16, 2023 05:36

jlf0dev reviewed Feb 16, 2023

View reviewed changes

eliykat requested changes Feb 17, 2023

View reviewed changes

coroiu added 10 commits February 17, 2023 11:28

[EC-775] feat: use proper treenode constructor

9e4b5b3

[EC-775] chore: remove unnecessary variable

81295c5

[EC-775] docs: add docs to relevant classes

a3b0059

[EC-775] chore: use existing function for searching tree

bdd3d80

[EC-775] fix: hide "new" button in trash view

5dd50c0

[EC-775] feat: add explicit handling for AllItems

ba558d8

[EC-775] fix: prune folderId when changing organization

7726d78

[EC-775] fix: properly use undefined instead of null

d7d39a6

[EC-775] chore: simplify setters using ternary operator

a8e66fc

[EC-775] feat: add static typing to type filter

284d64d

eliykat reviewed Feb 20, 2023

View reviewed changes

coroiu added 6 commits February 20, 2023 14:41

[EC-775] feat: use new All variable for collections

c274133

[EC-775] feat: return RouterLink compatible link from createRoute

7dbe8dc

[EC-775] feat: add ordId path support to createRoute

3a996e9

[EC-775] fix: interpret params differently in org vault

a69b064

This is needed due to how defaults used to work when using `state-in-code`. We really want to get rid of this type of logic going forward.

[EC-775] doc: clarify createRoute

61395dc

[EC-775] fix: better type typing

2529205

eliykat requested changes Feb 21, 2023

View reviewed changes

coroiu added 3 commits February 21, 2023 10:02

[EC-775] feat: remove support for path navigation

10a8b32

It's better that we circle back to this type of navigationt when we're working on the VVR and have more knowledge about how this is supposed to work.

[EC-775] fix: refactor bridge service to improve readability

e1803bf

Refactor follows feedback from PR review

Merge branch 'master' into EC-775-technical-dependency-refactor-vault…

c20cc6f

…-filters-to-be-routable

eliykat previously approved these changes Feb 21, 2023

View reviewed changes

eliykat added the needs-qa Marks a PR as requiring QA approval label Feb 21, 2023

Merge branch 'master' into EC-775-technical-dependency-refactor-vault…

319d469

…-filters-to-be-routable

coroiu dismissed eliykat’s stale review via 319d469 February 23, 2023 10:38

eliykat approved these changes Feb 23, 2023

View reviewed changes

Merge branch 'master' into EC-775-technical-dependency-refactor-vault…

72de73d

…-filters-to-be-routable

coroiu removed the needs-qa Marks a PR as requiring QA approval label Mar 6, 2023

coroiu merged commit ea66667 into master Mar 6, 2023

coroiu deleted the EC-775-technical-dependency-refactor-vault-filters-to-be-routable branch March 6, 2023 07:34


		const bridgeModel = new RoutedVaultFilterBridge(filter, legacyFilter, this);

		return bridgeModel;

Conversation

coroiu commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Objective

Code changes

Temporary compatibility layer

Screenshots

Before you submit

Uh oh!

jlf0dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coroiu Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlf0dev Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eliykat Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coroiu commented Feb 17, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coroiu commented Feb 13, 2023 •

edited

Loading

eliykat Feb 17, 2023 •

edited

Loading

coroiu Feb 17, 2023 •

edited

Loading

eliykat Feb 20, 2023 •

edited

Loading

jlf0dev Feb 16, 2023 •

edited

Loading

eliykat Feb 17, 2023 •

edited

Loading